Independent top-layer review on bb006241 (layer diff against #2153).

Staging beside the destination and validating before promotion correctly protects against encode/validation/cancellation failures (packages/producer/src/services/renderOrchestrator.ts:1532, :2950, :3040).

Blocker — packages/producer/src/services/render/artifactTransaction.ts:89: replacing an existing artifact is a two-syscall destination → backup, then staging → destination sequence (:90-92). Between those calls the destination does not exist, so concurrent readers can observe ENOENT; a process crash in that window leaves only the hidden backup. That is not atomic publication, which is the core contract of this PR. Use a replacement/recovery design that keeps a valid destination continuously addressable, and add a test/failure-injection case covering the handoff window rather than only final end states.

The layer checks are green aside from pending Graphite mergeability. This finding is independent of the parent #2153 verdict.

Verdict: REQUEST CHANGES
Reasoning: Validation and rollback are improved, but the promotion algorithm still exposes an absent-artifact window and cannot provide the advertised atomicity guarantee.

— Home

Reviewed at bb00624.

Clean, focused, well-scoped. Staging beside the destination on the same filesystem for atomic-rename semantics is the right shape, and assertReadableNonEmptyFile (non-empty, non-symlink, readable via 1-byte probe) is a thorough gate before promotion. The interaction with #2153's RenderQualityError throw is correct: the outer catch calls artifactTransaction.rollback() first via safeCleanup, so a strict-mode media failure never leaves a staged artifact orphaned. State machine is idempotent — rollback short-circuits when state isn't active, and commit's own inner recovery on second-rename failure stays in active so a follow-on rollback becomes a no-op restore.

Nits

• commit() embeds an inline backup-restore path (rmSync(destinationPath, ...) + renameSync(backupPath, destinationPath) on second-rename failure) that partially mirrors rollback()'s restore path with different conditions. Fine to keep for intent-clarity; a shared _restoreBackup() helper would trim it slightly. Non-blocking.

What I didn't verify

• The failed-second-rename recovery path — coverable only via a mocked filesystem (e.g. spying renameSync to throw once). Not exercised by the test suite; the logic reads correctly but is unverified in CI.
• Concurrent renders to the same outputPath: each transaction gets UUID-suffixed staging + backup, so intermediates are always valid single artifacts, but two racing commits pick a nondeterministic winner. Only a concern if the caller assumed linearization.

— Review by Rames D Jusso

R1 by Via

PR #2154 — fix(render): publish artifacts atomically
Verdict: 🔴 BLOCK — sustains Miguel's atomicity finding + adds resource-leak concern
Head: bb006241 on stack-base #2153@050fad62
Status: mergeable=unstable (Graphite check in-progress); CI green (regression + player-perf + preview-regression all green); prior reviews — miguel-heygen CHANGES_REQUESTED @ 20:07Z, james-russo-rames-d-jusso COMMENTED @ 20:11Z.

Summary of change
Introduces ArtifactTransaction (file/directory) that stages beside the destination, validates the staged artifact (lstat non-symlink non-empty + 1-byte fd probe; recursive for dirs), then commits via dest→backup + staging→dest two-rename sequence with inner backup-restore on second-rename failure. renderOrchestrator.executeRenderJob writes encode/assemble stages to stagedOutputPath, validates, does debug-copy + workDir cleanup, then commit() — then flips job.outputPath and status=complete. Catch path calls rollback() via safeCleanup.

Findings

🔴 [Sustain Miguel] Two-syscall publish is not atomic — violates the PR's own thesis — packages/producer/src/services/render/artifactTransaction.ts:89-92. commit() does renameSync(dest, backup) THEN renameSync(staging, dest). Between them the destination doesn't exist. Concurrent readers see ENOENT; a SIGKILL/host-crash/ENOSPC in the window leaves the caller-visible artifact gone and only a hidden .stem.hf-backup-<uuid> file remains, with no GC to find it. PR body says "atomically promote" — this delivers "validated, then near-atomic promote with a race window." Refute-attempt: with single-writer per-job semantics the concurrent-reader case is theoretical, and the dest-already-exists case only fires on retries (where losing the prior artifact is arguably acceptable). But the stated contract fails on both axes, and Node has fs.rename(staging, dest) as a single atomic call for FILES on both POSIX and Windows (Windows MoveFileEx with MOVEFILE_REPLACE_EXISTING). File path could be one-syscall; only the directory case genuinely needs a workaround (renameat2 RENAME_EXCHANGE Linux-only, or symlink-swap). Right fix: single-rename for files; document the directory-case tradeoff.

🟡 Orphaned staging/backup on abnormal exit — no GC — artifactTransaction.ts:106-112. rollback() only runs from the outer catch. SIGKILL, container-OOM, ENOSPC after staging bytes but before catch (e.g. inside runEncodeStage's own writers), power loss — all leave .stem.hf-staging-<uuid>{.ext} (and possibly .hf-backup-<uuid> if second-rename failed and inner recovery aborted) permanently in the output dir. No pruning at start-up, no age-based cleanup. Refute-attempt: if the parent dir is per-job ephemeral, orphans die with it. But dirname(outputPath) is the caller's output dir (typically shared across jobs, based on PRODUCER_RENDERS_DIR), not workDir — orphans accumulate. Suggest a startup sweep of .*.hf-{staging,backup}-* older than N hours in the same directory the transaction targets.

🟡 Double validation on happy path — orchestrator calls artifactTransaction.validate() at renderOrchestrator.ts:2950, then commit() at :3035 internally calls validate() again (artifactTransaction.ts:65). For png-sequence with thousands of frames, that's 2× lstat+open+read+close per frame. Non-blocking, but either drop the explicit call at :2950 or add a skipValidate flag / _validated guard.

⚪ Debug-copy from staging before commit — renderOrchestrator.ts:3019-3021 copies stagedOutputPath to workDir/output${videoExt} in debug mode. If the subsequent commit() fails and we roll back, the debug copy still lives in workDir. Probably fine (debug-only, workDir is inspection-mode by definition), noting for completeness.

⚪ "completed" checkpoint tag dropped — renderOrchestrator.ts:2955. Old code emitted observability.checkpoint("pipeline", "completed", ...); new code emits "artifact validated". Free-form string, not an enum, so no compile-time break — but Datadog dashboards / log-search alerts filtering on pipeline: completed will silently lose signal. Worth a heads-up + follow-up query update if any dashboards depend on it.

Peer-lens acknowledgment
Miguel's block already covers the primary atomicity thesis. Rames' code-clean LGTM-with-nits (state-machine idempotency, shared _restoreBackup() helper suggestion, uncovered second-rename-failure path, concurrent-writer nondeterminism) is accurate; my BLOCK sustains Miguel and adds the orphan-cleanup + double-validate not covered elsewhere.

Cross-checks

• Head SHA current: yes (bb00624189eac897a335eb69628b1d12b400f144)
• Prior reviewers: miguel-heygen (BLOCK), james-russo-rames-d-jusso (COMMENTED, effective LGTM w/ nits)
• Mergeability: unstable (Graphite mergeability_check in_progress; substantive CI green)
• POSIX vs Windows rename semantics handled: partial — Node renameSync normalizes across platforms (Windows uses MoveFileEx with MOVEFILE_REPLACE_EXISTING), so the 2-step is cross-platform-consistent but suboptimal for files (single-rename would be truly atomic on both). Directory-case is genuinely hard on either OS.
• Every publish site uses new atomic path: yes — grep of outputPath|stagedOutputPath in renderOrchestrator.ts at head confirms encode (:2880), assemble (:2938), debug-copy (:3019-3021) all read/write through stagedOutputPath; job.outputPath = outputPath (the public field) only assigned after commit().
• Test coverage: real (imports ./artifactTransaction.js, not aspirational). Covers 5 happy/rollback paths for file + directory. Missing: second-rename-failure recovery (Rames noted), concurrent-writer race, abnormal-exit orphan behavior.

R1 by Via

Re-reviewed current head aefb7fab46ad089a1a7e2be9d5f1f414851d09ae.

The layer-specific blocker from my prior review is addressed: file artifacts now publish with a single replacement rename, the failure-path test proves the old destination remains addressable, and the PR text/code explicitly narrow existing non-empty directory replacement to a recoverable (not atomic/crash-safe) handoff. I found no new blocker in this three-file layer.

I cannot approve the stack yet because required regression checks on this head are failing from the unresolved parent #2153 strict-by-default rollout/regressions. This review should become approval once the parent blocker is fixed and this head is rerun green.

Verdict: COMMENT (layer ready; stack blocked)
Reasoning: The artifact-transaction issue is fixed, but required inherited CI is red and the parent PR remains blocked.
— Magi

Re-reviewed current head 8fc6645c30dfea24618b7b34bd4d5bf15419c542 after the parent rewrite and full CI rerun.

The layer remains a narrow three-file artifact-publication change. File outputs validate in sibling staging and replace the destination with one rename; injected promotion failure leaves the old file continuously addressable. Existing non-empty directory replacement is explicitly documented as a recoverable, non-atomic handoff rather than overclaiming crash-safe visibility, and validation/cancellation paths preserve the prior destination. The orchestrator publishes completion only after commit and rolls staging back on every error path.

Focused artifact-transaction verification passed 6/6 tests. The inherited parent is now approved and green; this head has all Windows, preview, player, and eight regression shards green. The only live status is Graphite mergeability bookkeeping, not a test failure or code blocker.

Verdict: APPROVE
Reasoning: The original absent-destination window is removed for file artifacts, the directory limitation is accurately bounded, failure behavior is covered, and the complete code-bearing CI fleet is green on the resolved parent.

— Deepwork

The current-head hardening is good: private transaction directories are atomically reserved with mkdtempSync, descriptor-based validation closes the prior TOCTOU window, every encode/assemble/debug producer writes through stagedOutputPath, and the focused artifact transaction suite passes 7/7. The original file-replacement blocker is resolved.

There is one remaining concurrency correctness blocker in the existing-directory path.

At packages/producer/src/services/render/artifactTransaction.ts:135-145, rollback can delete another transaction's successfully committed directory. A deterministic interleaving is:

1. T1 renames the old destination to T1's backup at line 135.
2. During that gap, T2 observes no destination, takes the one-rename branch at lines 117-127, publishes its complete staging directory, and returns committed.
3. T1's staging rename at line 137 then fails because T2's non-empty directory occupies the destination.
4. T1's catch sees the destination at lines 141-143, recursively deletes it, and restores the old backup at line 144.

T2 has already reported success, but its complete artifact is removed and the destination silently reverts to the pre-render version. The collision-safe staging directories do not protect the shared destination, and concurrent direct/CLI renders can target the same PNG-sequence path.

Please serialize directory commits per destination, or make recovery ownership-aware so it never removes a destination that appeared after this transaction's handoff. Add a deterministic two-transaction interleaving test; current directory tests cover happy promotion and validation failure, but not second-rename/concurrent recovery.

Freshness: reviewed exact head f65003bd561c9df7f7cb1a079f710eaf57390b74; all current checks are green and there are zero unresolved prior threads.

Verdict: REQUEST CHANGES
Reasoning: Concurrent directory commits can make one transaction delete another transaction's already-successful output; recovery must not remove a destination it does not own.

— Deepwork